Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improve msbuild instance scoring #1545

Merged
merged 9 commits into from
Jul 12, 2019

Conversation

filipw
Copy link
Member

@filipw filipw commented Jul 7, 2019

We pick msbuild using a scoring system, by looking at VS msbuild instances, the msbuild bundled with OmniSharp and Mono if available.

Current status:

  • we always give preference to VS msbuild instance
  • we use the bundled msbuild only if VS/Mono is not available
  • minimum Mono is 5.2.x

This worked well when VS 2017 was the latest one - now that VS 2019 is there, and .NET Core 3.0 is there, things break down a bit.
Currently, when the user has VS 2017 installed, we pick MsBuild from there, however that doesn't work with .NET Core 3.0.
We might also sometimes pick VS 2017 over VS 2019.

New status:

  • we still give highest preference to VS
  • additionally we score extra points for msbuild 16 (VS 2019, msbuild bundled in OmniSharp, new Mono)
  • minimum Mono is 5.18.x (which is inline with the fact we moved to 472)

With this change, since the msbuild bundled with OmniSharp is v16, it gets picked over VS 2017.
VS 2017 is never picked over VS 2019 anymore.
If the user has VS 2019 installed, we will still pick that one over our bundled version, as it gets scored higher.

The consequence of this change is that we will actually never pick VS2017 anymore on Windows, since we'll always prefer our bundled version. I think this is fine, since I would expect OmniSharp to always be aligned with the latest VS, and if that's not found, to use its own bundled msbuild.

Fixes #1541
It should fix some other issues / behavioral inconsistencies we have.

@rchande
Copy link

rchande commented Jul 8, 2019

Curious to know what validation you did for this change, eg what os/vs/sdk matrix did you test? :)

@filipw
Copy link
Member Author

filipw commented Jul 8, 2019

I tested on:

  • Windows with VS2017 + OmniSharp (picks standalone)
  • Windows with VS2019 + OmniSharp (picks VS2019)
  • Windows with VS2017 + VS2017 + OmniSharp (picks VS2019)
  • macOS with Mono 5.18.1 + OmniSharp (picks mono)

At the end it's a small change really, we just score msbuild 16 higher now.

There might be a negative side effect if you relied on VS2017 msbuild - now you will be getting standalone. But as soon as you install VS2019, we pick the VS instance again.

And as we have seen many people complain when VS2017 is picked over standalone instance cause .NET Core 3.0 doesn't work then.

@rchande
Copy link

rchande commented Jul 8, 2019

In the past, we've talked @nguererra about adding an option to let you select the build in msbuild over the one in VS. Now that we're flipping the default, do you think it makes sense for us to add an option to request VS 2017 msbuild over the built in one? That might help people who encounter

There might be a negative side effect if you relied on VS2017 msbuild -

I know we've had people mention that they're unable to upgrade to later VS versions.

@david-driscoll
Copy link
Member

We could maybe expose the ability to configure MS build or to change the msbuild version at runtime.

@filipw
Copy link
Member Author

filipw commented Jul 10, 2019

makes sense

@akshita31
Copy link
Contributor

@filipw Are there any more changes that you would be making here? Also will we need a change log for this?
We were thinking to merge this and put it out for the C# extension release next week. Thoughts ?

@filipw
Copy link
Member Author

filipw commented Jul 10, 2019

I will just add the ability to manually override the path to a predefined one (or to manually choose the preferred flavor VS2019, VS2017, Mono, StandAlone) and it will be good to go.
I should have some time tomorrow to finish this.

We'll handle changelog separately since there is a bunch of other changes that needs describing anyway so we'll just to it in one go.

@rchande
Copy link

rchande commented Jul 10, 2019

Thanks, lgtm!

@filipw
Copy link
Member Author

filipw commented Jul 11, 2019

@rchande @david-driscoll as discussed, I added an option to provide custom MSBuild Path.

It's done via msbuild section in the omnisharp.json config.
For example, to force VS2017 (name is for display purposes only and is optional):

{
    "MSBuild": {
        "MSBuildOverride": {
            "MSBuildPath": "C:\\Program Files (x86)\\Microsoft Visual Studio\\2017\\Enterprise\\MSBuild\\15.0\\Bin",
            "Name": "filip's msbuild"
        }
    }
}

To force a specific Mono:

{
    "MSBuild": {
        "MSBuildOverride": {
            "MSBuildPath": "~/mono/5.18.1/lib/mono/msbuild/15.0/bin",
            "Name": "filip's msbuild"
        }
    }
}

And more advanced example, with custom property overrides:

{
    "MSBuild": {
        "MSBuildOverride": {
            "MSBuildPath": "C:\\code\\msbuild\\Current\\Bin",
            "Name": "filip's msbuild"
             "PropertyOverrides": {
                 "MSBuildToolsPath": "C:\\code\\msbuild\\Current\\Bin",
                 "MSBuildExtensionsPath": "C:\\code\\msbuild"
                 "CscToolPath": "C:\\code\\msbuild\\Current\\Bin\\Roslyn",
                 "CscToolExe": "csc.exe"
             }
        }
    }
}

The override from the user is always picked over any other candidate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.NET Core 3.0 preview 6, unreferenced System error.
4 participants